Codecov Report

Merging #2075 (aeed39c) into main (3d4d8b5) will not change coverage.
The diff coverage is 100.00%.

❗ Current head aeed39c differs from pull request most recent head ce482c4. Consider uploading reports for the commit ce482c4 to get more accurate results

@@           Coverage Diff           @@
##             main    #2075   +/-   ##
=======================================
  Coverage   93.03%   93.03%           
=======================================
  Files         154      154           
  Lines        5976     5976           
  Branches     1246     1246           
=======================================
  Hits         5560     5560           
  Misses        416      416           

This seems to be inconsistent now.

• interface SpanProcessor#onStart gets a api.Span now
• implementations like BatchSpanProcessor and MultiSpanProcessor still use the class Span
• interface SpanProcessor#onEnd gets a ReadableSpan (class, not interface)

Please note the subtle difference in the APIs, e.g. there is ReadableSpan#ended or ReadableSpan#resource which are also available in class Span but not in api.Span.

• interface SpanProcessor#onStart gets a api.Span now

yes

• implementations like BatchSpanProcessor and MultiSpanProcessor still use the class Span

Class span implements the Span interface expected by the SpanProcessor interface so this is OK. The SpanProcessor will only ever receive spans created by ourselves so we know it will always receive this concrete implementation. In any case, I've changed it to use the interface instead.

• interface SpanProcessor#onEnd gets a ReadableSpan (class, not interface)

It is an interface https://github.com/dyladan/opentelemetry-js/blob/span-processor-interface/packages/opentelemetry-tracing/src/export/ReadableSpan.ts#L29

Please note the subtle difference in the APIs, e.g. there is ReadableSpan#ended or ReadableSpan#resource which are also available in class Span but not in api.Span.

As long as an implementation which satisfies the interface is used, it should work.

The SpanProcessor will only ever receive spans created by ourselves so we know it will always receive this concrete implementation. In any case, I've changed it to use the interface instead.

Well that's true but if you want to access any detail from class Span it requires a cast now. Typescript will no longer complain if e.g. a NoopSpan is given to onStart().

The existing SpanProcessors in this repo have no need for onStart() but someone might create an improved processor where onStart() has a real implementation which wants to use the rich data from class Span.

I'm not sure if we really want to remove this functionality to avoid issues caused by a mixed version setup.

This might not be needed once #2073 is merged anyway

This might not be needed once #2073 is merged anyway

it was merged, does it mean it is not needed now or ?

Probably. I'll close for now but not delete the branch until we release with pinned versions and ensure the problem is actually fixed.